feat: support batched json rpc requests #675
Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
📝 WalkthroughWalkthroughAdded batch JSON-RPC support to the HTTP layer. Introduced Possibly related PRs
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (5)📚 Learning: 2025-10-21T14:00:54.642ZApplied to files:
📚 Learning: 2025-11-20T08:57:07.217ZApplied to files:
📚 Learning: 2025-11-21T11:03:26.756ZApplied to files:
📚 Learning: 2025-11-19T12:55:48.931ZApplied to files:
📚 Learning: 2025-11-24T08:45:11.154ZApplied to files:
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.toml(1 hunks)magicblock-aperture/Cargo.toml(1 hunks)magicblock-aperture/src/requests/http/mod.rs(3 hunks)magicblock-aperture/src/requests/mod.rs(1 hunks)magicblock-aperture/src/server/http/dispatch.rs(4 hunks)magicblock-aperture/tests/batches.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T14:00:54.642Z
Learnt from: bmuddha
Repo: magicblock-labs/magicblock-validator PR: 578
File: magicblock-aperture/src/requests/websocket/account_subscribe.rs:18-27
Timestamp: 2025-10-21T14:00:54.642Z
Learning: In magicblock-aperture account_subscribe handler (src/requests/websocket/account_subscribe.rs), the RpcAccountInfoConfig fields data_slice, commitment, and min_context_slot are currently ignored—only encoding is applied. This is tracked as technical debt in issue #579: https://github.com/magicblock-labs/magicblock-validator/issues/579
Applied to files:
magicblock-aperture/src/server/http/dispatch.rs
🧬 Code graph analysis (1)
magicblock-aperture/src/server/http/dispatch.rs (2)
magicblock-aperture/src/requests/payload.rs (3)
encode(61-79)encode(105-115)encode(120-133)magicblock-aperture/src/requests/http/mod.rs (2)
extract_bytes(67-100)parse_body(49-64)
🔇 Additional comments (2)
magicblock-aperture/Cargo.toml (1)
72-72: LGTM!The addition of
reqwestas a dev-dependency is appropriate for the new batch request tests intests/batches.rs.magicblock-aperture/src/server/http/dispatch.rs (1)
110-112: LGTM! Access control headers on error responses.Good catch to ensure CORS headers are set on error responses. This prevents CORS-related failures in browser clients when requests fail.
thlorenz
left a comment
thlorenz
left a comment
There was a problem hiding this comment.
LGTM (with a note around error returns) + please consider removing that handrolled JSON generator (possibley run a quick test regarding perf).
| } | ||
| .trim_ascii_start(); | ||
| // Hacky/cheap way to detect single request vs an array of requests | ||
| if body_bytes.first().map(|&b| b == b'{').unwrap_or_default() { |
There was a problem hiding this comment.
What if the user sends an invalid payload, i.e. "Hello world" (which is valid JSON).
At that point we'd already try to interpret as an array and the error message might be misleading.
Maybe we should also check specifically for [ and have a third branch which errors with a message that an array or object is expected.
I ran such an invalid request against our current implementation and it returns:
{
"jsonrpc": "2.0",
"error": {
"code": -32700,
"message": "error parsing request body: invalid type: string \"Hello World\", expected struct JsonRequest at line 1 column 13\n\n\tlo World\"\n\t........^\n"
}
}Running against stock solana validator returns:
{
"jsonrpc": "2.0",
"error": {
"code": -32700,
"message": "Parse error"
},
"id": null
}So we already do a better job here it seems. I double checked that we still return the same message on this branch (while writing this comment), so it seems fine either way, just something to consider to make things more explicit.
| let response = self.process(&mut request).await; | ||
| // Handle any errors from the execution stage | ||
| let response = unwrap!(response, Some(&request.id)); | ||
| let (response, id) = match request { |
There was a problem hiding this comment.
Why are we handrolling JSON generators? Why not use a proper crate to do this?
This code in the middle of request handling is quite verbose and confused me to the point where I thought this is a JSON parser actually.
We should use serde_json instead to build up a proper request and then serialize
Or we could use the json! macro.
If this is done for speed then we should be sure the difference is worth it.
serde_json does pretty much what your code is doing, just more battle tested and most likely pretty much as performant.
There was a problem hiding this comment.
It's done for speed mostly yes, each handler returns a byte arrray of already serialized bodies, in order to use more expensive json serializer, we need to convert those bodies back to json structures and then serialize it back, which is 3 times more work than this implementation does.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.